header to metadata: add base64 encode and protobuf value type by yangminzhu · Pull Request #7796 · envoyproxy/envoy

yangminzhu · 2019-08-01T03:57:12Z

Signed-off-by: Yangmin Zhu ymzhu@google.com

For the first point in #7771 for converting arbitrary protobuf value from header to metadata.

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level: Low
Testing: Unit tests
Docs Changes: Updated version_history.rst
Release Notes: Updated version_history.rst
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu · 2019-08-01T04:04:37Z

For the second point, just a quick question, do we prefer to add a new metadata_to_header filter or expand the header_to_metadata API to support it?

kyessenov · 2019-08-01T04:09:15Z

Can something be done to avoid serializing the metadata on every request, and do it only once during filter construction? That's the critical part in the existing istio implementation.

kyessenov · 2019-08-01T04:10:26Z

cc @mandarjog

lizan · 2019-08-01T04:12:12Z

api/envoy/config/filter/http/header_to_metadata/v2/header_to_metadata.proto

+    // `Base64Url <https://tools.ietf.org/html/rfc4648#section-5>`_ encoded string of
+    // serialized `protobuf.Value
+    // <https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/struct.proto#L62>`_
+    BASE64URL_SERIALIZED = 2;


This is counter intuitive from API stand point without reading the comment above, from reading the implementation, I'd say make this VALUE or PROTOBUF_VALUE, and have another enum for the encoding to express it, since which base64 or not is orthogonal to value type here, WDYT?

That looks a cleaner API though I imagine the base64 option is likely to be always used only with PROTOBUF_VALUE?

I'm fine with the suggestion, @rgs1 WDYT?

It is possible for someone to use STRING with BASE64 with some non header safe character sets (UTF-8), and when you add more binary types here it will help anyway.

yeah i like Lizan's suggestion (PROTOBUF_VALUE).

Thanks, updated.

yangminzhu · 2019-08-01T04:24:49Z

@kyessenov Hmm, not sure I understand your question. Could you clarify a little bit? Thanks. I thought the header value is only known on request so how can we do the serialization only during filter construction?

One thing I can think of right now is to use a cache to avoid the serialization on every request. Is this what you're suggesting?

kyessenov · 2019-08-01T04:27:08Z

@yangminzhu The filter I referenced sends node metadata, which is always the same across all requests. For example, pod labels.

yangminzhu · 2019-08-01T04:32:43Z

@kyessenov I see, Are you referring to the client side sending the metadata via header? I think that is not covered in this PR and will be solved with the new metadata_to_header filter. Thanks for raising the concern, I think the API of the metadata_to_header filter should be able to specify it's from the node metadata, and in this case we can only serialize it once and use it everytime.

This PR is about the server side decoding the header, Is there anything else we should take care of here? thanks!

lizan · 2019-08-01T04:34:21Z

@yangminzhu @kyessenov take a look on #2394 while it is not completely standardized though it is partially implemented. cc @soya3129 for current status.

lizan · 2019-08-01T04:34:56Z

IIUC it allows per connection metadata exchange as well as per stream metadata exchange.

kyessenov · 2019-08-01T04:38:11Z

@lizan Yes, we're are aware of H2 metadata. It's very cool, but unfortunately the world has not even moved to H2 yet, so it'll cause issues with inter-operability with other proxies (or even gRPC).

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc

kyessenov · 2019-08-01T06:13:32Z

@yangminzhu This is fine for extracting a request header to metadata. I'm still not entirely sure how this works for extracting a response headers from an upstream host, to be usable as a replacement for the istio filter.

rgs1 · 2019-08-01T15:46:52Z

@yangminzhu This is fine for extracting a request header to metadata. I'm still not entirely sure how this works for extracting a response headers from an upstream host, to be usable as a replacement for the istio filter.

The h-t-m filter works for both request/response headers, fwiw.

Also, do we need a separate metadata-to-header filter or could we have that functionality in this filter as well?

kyessenov · 2019-08-01T17:56:09Z

@rgs1 Gotcha, I didn't see response rules at first.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-02T02:48:22Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

yangminzhu · 2019-08-02T02:52:49Z

@lizan @rgs1 @kyessenov PTAL, the coverage seems failing for some network error, thank you.

zuercher

This seems very useful. Thanks for looking into it. To start with, I have a question about the API, once we get that nailed down I'll look at the rest more closely.

api/envoy/config/filter/http/header_to_metadata/v2/header_to_metadata.proto

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-02T22:42:30Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-02T22:46:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-03T03:00:05Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

docs/root/intro/version_history.rst

htuch

@envoyproxy/api-shepherds API LGTM

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-05T18:01:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

lizan

great, thanks. just one nit.

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc

test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

repokitteh-read-only · 2019-08-05T23:15:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7796 was synchronize by yangminzhu.

see: more, trace.

yangminzhu · 2019-08-06T21:14:57Z

@htuch Would you mind to approve for the API again? I pushed a new commit to resolve some other comments, no new changes in API. Thank you.

lizan · 2019-08-06T21:20:06Z

No API changes since last API review, just merged.

add base64url serialized type

1461574

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu requested a review from zuercher as a code owner August 1, 2019 03:57

lizan self-assigned this Aug 1, 2019

lizan reviewed Aug 1, 2019

View reviewed changes

yangminzhu changed the title ~~add base64url serialized type~~ header to metadata: add base64url serialized type Aug 1, 2019

kyessenov reviewed Aug 1, 2019

View reviewed changes

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc Outdated Show resolved Hide resolved

kyessenov reviewed Aug 1, 2019

View reviewed changes

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc Outdated Show resolved Hide resolved

add ValueEncode enum and encode field

c9aa850

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

zuercher reviewed Aug 2, 2019

View reviewed changes

api/envoy/config/filter/http/header_to_metadata/v2/header_to_metadata.proto Outdated Show resolved Hide resolved

zuercher self-assigned this Aug 2, 2019

update comments and use base64

bbd5f7b

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

yangminzhu changed the title ~~header to metadata: add base64url serialized type~~ header to metadata: add base64 encode and protobuf value type Aug 2, 2019

fix doc

05d62f6

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 2, 2019

View reviewed changes

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc Outdated Show resolved Hide resolved

add TODO

cc17e54

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 3, 2019

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

htuch reviewed Aug 5, 2019

View reviewed changes

update doc

f94545c

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan reviewed Aug 5, 2019

View reviewed changes

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc Outdated Show resolved Hide resolved

kyessenov reviewed Aug 5, 2019

View reviewed changes

test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc Outdated Show resolved Hide resolved

address comments

3a8cda6

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

lizan approved these changes Aug 5, 2019

View reviewed changes

lizan merged commit 9e552c5 into envoyproxy:master Aug 6, 2019

yangminzhu deleted the metadata branch August 6, 2019 21:20

Conversation

yangminzhu commented Aug 1, 2019

Uh oh!

yangminzhu commented Aug 1, 2019

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

lizan Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

yangminzhu Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

rgs1 Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

yangminzhu Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

yangminzhu commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

yangminzhu commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Aug 1, 2019

Uh oh!

lizan commented Aug 1, 2019

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

Uh oh!

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

rgs1 commented Aug 1, 2019

Uh oh!

kyessenov commented Aug 1, 2019

Uh oh!

repokitteh-read-only bot commented Aug 2, 2019

Uh oh!

yangminzhu commented Aug 2, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 2, 2019

Uh oh!

repokitteh-read-only bot commented Aug 2, 2019

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 3, 2019

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Aug 5, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 5, 2019

Uh oh!

yangminzhu commented Aug 6, 2019

Uh oh!

lizan commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

yangminzhu commented Aug 1, 2019 •

edited

Loading

yangminzhu commented Aug 1, 2019 •

edited

Loading